test: fix argument computation in embedtest #49506

joyeecheung · 2023-09-05T23:47:29Z

bootstrap: do not expand argv1 for snapshots

To avoid capturing build machine states into the snapshot

test: fix argument computation in embedtest

There were a few bugs in the original test that went unnoticed
because with the bug the test did not actually get run anymore.
This patch fixes the argument computation by accounting filtering
of the arguments, and uses spawnSyncAndExit{WithoutError} in
the test to enable better logging when the test fails.

build: run embedtest using node executable

We should use the node executable to run this test, instead of
counting on embedtest, the binary being tested, as the test runner.
Otherwise bugs can go unnoticed if the embedtest binary itself
does not work correctly.

Fixes: #49501

nodejs-github-bot · 2023-09-06T14:05:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53767/

To avoid capturing build machine states into the snapshot

There were a few bugs in the original test that went unnoticed because with the bug the test did not actually get run anymore. This patch fixes the argument computation by accounting filtering of the arguments, and uses spawnSyncAndExit{WithoutError} in the test to enable better logging when the test fails.

We should use the node executable to run this test, instead of counting on embedtest, the binary being tested, as the test runner. Otherwise bugs can go unnoticed if the embedtest binary itself does not work correctly.

nodejs-github-bot · 2023-09-08T11:06:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53813/

nodejs-github-bot · 2023-09-11T11:24:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53885/

joyeecheung · 2023-09-11T11:24:47Z

Can I have some reviews please? @nodejs/startup @nodejs/cpp-reviewers

test/embedding/embedtest.cc

nodejs-github-bot · 2023-09-13T14:47:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53932/

joyeecheung · 2023-09-15T07:45:35Z

@nodejs/build-files @nodejs/cpp-reviewers can I get some reviews please?

RaisinTen · 2023-09-15T08:15:39Z

test/embedding/embedtest.cc

      loadenv_ret = node::LoadEnvironment(env, node::StartExecutionCallback{});
-    } else {
+    } else if (is_building_snapshot) {


Should we sync the code in the embedding docs (

node/doc/api/embedding.md

Lines 112 to 166 in 6a489df

int RunNodeInstance(MultiIsolatePlatform* platform,

const std::vector<std::string>& args,

const std::vector<std::string>& exec_args) {

int exit_code = 0;

// Setup up a libuv event loop, v8::Isolate, and Node.js Environment.

std::vector<std::string> errors;

std::unique_ptr<CommonEnvironmentSetup> setup =

CommonEnvironmentSetup::Create(platform, &errors, args, exec_args);

if (!setup) {

for (const std::string& err : errors)

fprintf(stderr, "%s: %s\n", args[0].c_str(), err.c_str());

return 1;

}

Isolate* isolate = setup->isolate();

Environment* env = setup->env();

{

Locker locker(isolate);

Isolate::Scope isolate_scope(isolate);

HandleScope handle_scope(isolate);

// The v8::Context needs to be entered when node::CreateEnvironment() and

// node::LoadEnvironment() are being called.

Context::Scope context_scope(setup->context());

// Set up the Node.js instance for execution, and run code inside of it.

// There is also a variant that takes a callback and provides it with

// the `require` and `process` objects, so that it can manually compile

// and run scripts as needed.

// The `require` function inside this script does *not* access the file

// system, and can only load built-in Node.js modules.

// `module.createRequire()` is being used to create one that is able to

// load files from the disk, and uses the standard CommonJS file loader

// instead of the internal-only `require` function.

MaybeLocal<Value> loadenv_ret = node::LoadEnvironment(

env,

"const publicRequire ="

" require('node:module').createRequire(process.cwd() + '/');"

"globalThis.require = publicRequire;"

"require('node:vm').runInThisContext(process.argv[1]);");

if (loadenv_ret.IsEmpty()) // There has been a JS exception.

return 1;

exit_code = node::SpinEventLoop(env).FromMaybe(1);

// node::Stop() can be used to explicitly stop the event loop and keep

// further JavaScript from running. It can be called from any thread,

// and will act like worker.terminate() if called from another thread.

node::Stop(env);

}

return exit_code;

}

) too?

Maybe, the code in there would continue to work, the snapshot capability and the snapshot testing code was added by #45888, so probably out of scope of this PR.

The doc has a reference to this file in

node/doc/api/embedding.md

Line 24 in 6a489df

The full code can be found [in the Node.js source tree][embedtest.cc].

, so I thought these were supposed to be in sync but this too lgtm if that's not needed

nodejs-github-bot · 2023-09-15T11:19:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53962/

To avoid capturing build machine states into the snapshot PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

There were a few bugs in the original test that went unnoticed because with the bug the test did not actually get run anymore. This patch fixes the argument computation by accounting filtering of the arguments, and uses spawnSyncAndExit{WithoutError} in the test to enable better logging when the test fails. PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

We should use the node executable to run this test, instead of counting on embedtest, the binary being tested, as the test runner. Otherwise bugs can go unnoticed if the embedtest binary itself does not work correctly. PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung · 2023-09-17T22:07:07Z

CI was green although the bot still had trouble communicating the green result back to GitHub so it got stuck as "pending. Landed it manually in cdcb01a...1995eca.

To avoid capturing build machine states into the snapshot PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

There were a few bugs in the original test that went unnoticed because with the bug the test did not actually get run anymore. This patch fixes the argument computation by accounting filtering of the arguments, and uses spawnSyncAndExit{WithoutError} in the test to enable better logging when the test fails. PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

We should use the node executable to run this test, instead of counting on embedtest, the binary being tested, as the test runner. Otherwise bugs can go unnoticed if the embedtest binary itself does not work correctly. PR-URL: #49506 Fixes: #49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

To avoid capturing build machine states into the snapshot PR-URL: nodejs#49506 Fixes: nodejs#49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

There were a few bugs in the original test that went unnoticed because with the bug the test did not actually get run anymore. This patch fixes the argument computation by accounting filtering of the arguments, and uses spawnSyncAndExit{WithoutError} in the test to enable better logging when the test fails. PR-URL: nodejs#49506 Fixes: nodejs#49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

We should use the node executable to run this test, instead of counting on embedtest, the binary being tested, as the test runner. Otherwise bugs can go unnoticed if the embedtest binary itself does not work correctly. PR-URL: nodejs#49506 Fixes: nodejs#49501 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Sep 5, 2023

joyeecheung mentioned this pull request Sep 5, 2023

test: fix embedtest not running code #49502

Closed

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2023

joyeecheung added 3 commits September 8, 2023 13:01

bootstrap: do not expand argv1 for snapshots

a779b0d

To avoid capturing build machine states into the snapshot

build: run embedtest using node executable

8b9b658

We should use the node executable to run this test, instead of counting on embedtest, the binary being tested, as the test runner. Otherwise bugs can go unnoticed if the embedtest binary itself does not work correctly.

joyeecheung force-pushed the embedtest branch from 9017e9d to 8b9b658 Compare September 8, 2023 11:04

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2023

This was referenced Sep 9, 2023

CI Reliability 2023-09-09 nodejs/reliability#660

Open

CI Reliability 2023-09-10 nodejs/reliability#661

Open

CI Reliability 2023-09-11 nodejs/reliability#662

Open

targos reviewed Sep 11, 2023

View reviewed changes

test/embedding/embedtest.cc Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Sep 12, 2023

CI Reliability 2023-09-12 nodejs/reliability#663

Open

19 tasks

fixup! test: fix argument computation in embedtest

fbf82ab

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 13, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 13, 2023

This was referenced Sep 14, 2023

CI Reliability 2023-09-14 nodejs/reliability#665

Open

CI Reliability 2023-09-15 nodejs/reliability#666

Open

RaisinTen reviewed Sep 15, 2023

View reviewed changes

RaisinTen approved these changes Sep 15, 2023

View reviewed changes

github-actions bot mentioned this pull request Sep 16, 2023

CI Reliability 2023-09-16 nodejs/reliability#667

Open

25 tasks

jasnell approved these changes Sep 16, 2023

View reviewed changes

github-actions bot mentioned this pull request Sep 17, 2023

CI Reliability 2023-09-17 nodejs/reliability#668

Open

32 tasks

joyeecheung closed this Sep 17, 2023

github-actions bot mentioned this pull request Sep 18, 2023

CI Reliability 2023-09-18 nodejs/reliability#669

Open

33 tasks

This was referenced Sep 28, 2023

v20.7.1 proposal #49917

Closed

v20.8.0 proposal #49932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix argument computation in embedtest #49506

test: fix argument computation in embedtest #49506

joyeecheung commented Sep 5, 2023 •

edited

Loading

nodejs-github-bot commented Sep 6, 2023

nodejs-github-bot commented Sep 8, 2023

nodejs-github-bot commented Sep 11, 2023

joyeecheung commented Sep 11, 2023

nodejs-github-bot commented Sep 13, 2023

joyeecheung commented Sep 15, 2023

RaisinTen Sep 15, 2023

joyeecheung Sep 15, 2023 •

edited

Loading

RaisinTen Sep 15, 2023

nodejs-github-bot commented Sep 15, 2023

joyeecheung commented Sep 17, 2023 •

edited

Loading

	int RunNodeInstance(MultiIsolatePlatform* platform,
	const std::vector<std::string>& args,
	const std::vector<std::string>& exec_args) {
	int exit_code = 0;

	// Setup up a libuv event loop, v8::Isolate, and Node.js Environment.
	std::vector<std::string> errors;
	std::unique_ptr<CommonEnvironmentSetup> setup =
	CommonEnvironmentSetup::Create(platform, &errors, args, exec_args);
	if (!setup) {
	for (const std::string& err : errors)
	fprintf(stderr, "%s: %s\n", args[0].c_str(), err.c_str());
	return 1;
	}

	Isolate* isolate = setup->isolate();
	Environment* env = setup->env();

	{
	Locker locker(isolate);
	Isolate::Scope isolate_scope(isolate);
	HandleScope handle_scope(isolate);
	// The v8::Context needs to be entered when node::CreateEnvironment() and
	// node::LoadEnvironment() are being called.
	Context::Scope context_scope(setup->context());

	// Set up the Node.js instance for execution, and run code inside of it.
	// There is also a variant that takes a callback and provides it with
	// the `require` and `process` objects, so that it can manually compile
	// and run scripts as needed.
	// The `require` function inside this script does not access the file
	// system, and can only load built-in Node.js modules.
	// `module.createRequire()` is being used to create one that is able to
	// load files from the disk, and uses the standard CommonJS file loader
	// instead of the internal-only `require` function.
	MaybeLocal<Value> loadenv_ret = node::LoadEnvironment(
	env,
	"const publicRequire ="
	" require('node:module').createRequire(process.cwd() + '/');"
	"globalThis.require = publicRequire;"
	"require('node:vm').runInThisContext(process.argv[1]);");

	if (loadenv_ret.IsEmpty()) // There has been a JS exception.
	return 1;

	exit_code = node::SpinEventLoop(env).FromMaybe(1);

	// node::Stop() can be used to explicitly stop the event loop and keep
	// further JavaScript from running. It can be called from any thread,
	// and will act like worker.terminate() if called from another thread.
	node::Stop(env);
	}

	return exit_code;
	}

test: fix argument computation in embedtest #49506

test: fix argument computation in embedtest #49506

Conversation

joyeecheung commented Sep 5, 2023 • edited Loading

bootstrap: do not expand argv1 for snapshots